feat: Add closest point and distance computation to SurfaceBounds
#3990
Conversation
WalkthroughIn this extensive pull request, the Acts library undergoes a significant refactoring of surface bounds and boundary tolerance handling. The changes primarily focus on simplifying method signatures, removing boundary tolerance complexity, and introducing more consistent geometric computation methods across various surface bound classes. Key modifications include removing unnecessary header inclusions, streamlining Changes
Sequence DiagramsequenceDiagram
participant SurfaceBounds
participant BoundsBefore as Bounds (Before)
participant BoundsAfter as Bounds (After)
SurfaceBounds->>BoundsBefore: Old inside(position, tolerance)
BoundsBefore-->>SurfaceBounds: Complex tolerance check
SurfaceBounds->>BoundsAfter: New inside(position)
BoundsAfter-->>SurfaceBounds: Simplified position check
SurfaceBounds->>BoundsAfter: New closestPoint(position, metric)
BoundsAfter-->>SurfaceBounds: Closest point calculation
Possibly related PRs
Suggested Labels
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
andiwand
changed the title
SurfaceBounds SurfaceBounds
|
|
|
After debugging today I think |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
Core/src/Surfaces/DiamondBounds.cpp (1)
Line range hint
35-63: Duplicate code, a path to the dark side it is!Extract the vertex calculation to a private method, we must. Repeated three times this code is - in inside(), closestPoint(), and vertices().
Create a private method, you should:
+private: + std::array<Vector2, 6> computeVertices() const { + double x1 = get(DiamondBounds::eHalfLengthXnegY); + double y1 = get(DiamondBounds::eHalfLengthYneg); + double x2 = get(DiamondBounds::eHalfLengthXzeroY); + double y2 = 0.; + double x3 = get(DiamondBounds::eHalfLengthXposY); + double y3 = get(DiamondBounds::eHalfLengthYpos); + return {Vector2{-x1, -y1}, {x1, -y1}, {x2, y2}, + {x3, y3}, {-x3, y3}, {-x2, y2}}; + }Then use it in all three methods:
bool DiamondBounds::inside(const Vector2& lposition) const { - // Vertices starting at lower left... - double x1 = get(DiamondBounds::eHalfLengthXnegY); - // ... [removed duplicate code] - Vector2 vertices[] = {{-x1, -y1}, {x1, -y1}, {x2, y2}, - {x3, y3}, {-x3, y3}, {-x2, y2}}; + auto vertices = computeVertices(); return detail::VerticesHelper::isInsidePolygon(lposition, vertices); }Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp (1)
Line range hint
87-96: A disturbance in the Force, I sense!The bounding box, properly updated it is not. Called is makeBoundingBox(), but its result, assigned it is not.
Fix this issue, you must:
template <int N> ConvexPolygonBounds<N>::ConvexPolygonBounds(const value_array& values) noexcept( false) - : m_vertices(), m_boundingBox(0., 0.) { + : m_vertices() { for (std::size_t i = 0; i < N; i++) { m_vertices[i] = Vector2(values[2 * i], values[2 * i + 1]); } - makeBoundingBox(m_vertices); + m_boundingBox = makeBoundingBox(m_vertices); checkConsistency(); }
🧹 Nitpick comments (28)
Tests/Benchmarks/BoundaryToleranceBenchmark.cpp (2)
25-26: Improve readability with named constants, we should.More readable and maintainable, the code would be, if the magic numbers in a structured way we defined. Hmmmm.
Like this, it should look:
+ // Define trapezoid vertices + const double left_bottom_x = 0.4; + const double right_bottom_x = 0.6; + const double left_top_x = 0.2; + const double right_top_x = 0.8; + const double bottom_y = 0.25; + const double top_y = 0.75; + ConvexPolygonBounds<PolygonDynamic> poly( - {{0.4, 0.25}, {0.6, 0.25}, {0.8, 0.75}, {0.2, 0.75}}); + {{left_bottom_x, bottom_y}, + {right_bottom_x, bottom_y}, + {right_top_x, top_y}, + {left_top_x, top_y}});
107-114: Strong with the Force, these changes are.Wisely have you adapted the benchmark to use the new
insidemethod. Clear and purposeful, the tests remain, while embracing the new ways of the ConvexPolygonBounds. Hmmmm.A suggestion I have, though minor it may be. Consider grouping the test cases by their nature, we should:
+ // Test fixed points run_bench([&] { return poly.inside(center, check); }, num_inside_points, "Center"); run_bench([&] { return poly.inside(edge_inside, check); }, num_inside_points, "Inside edge"); run_bench([&] { return poly.inside(edge_outside, check); }, num_outside_points, "Outside edge"); run_bench([&] { return poly.inside(far_away, check); }, num_outside_points, "Far away"); + // Test random points std::vector<Vector2> points(num_outside_points); std::generate(points.begin(), points.end(), random_point); run_bench_with_inputs( [&](const auto& point) { return poly.inside(point, check); }, points, "Random");Also applies to: 120-121
Core/src/Surfaces/EllipseBounds.cpp (2)
46-54: Improve readability of mathematical conditions, you must.Correct the implementation is, but clearer it could be made. Break down the complex condition into named variables, we should. Hmmmm.
Like this, it should look:
bool EllipseBounds::inside(const Vector2& lposition) const { double phi = detail::radian_sym(VectorHelpers::phi(lposition) - get(eAveragePhi)); - return (-get(eHalfPhiSector) <= phi) && (phi < get(eHalfPhiSector)) && - (square(lposition[eBoundLoc0] / get(eInnerRx)) + - square(lposition[eBoundLoc1] / get(eInnerRy))) >= 1 && - (square(lposition[eBoundLoc0] / get(eOuterRx)) + - square(lposition[eBoundLoc1] / get(eOuterRy))) < 1; + // Check if angle is within sector bounds + bool withinPhiSector = (-get(eHalfPhiSector) <= phi) && + (phi < get(eHalfPhiSector)); + + // Check if point is outside inner ellipse + double innerEllipseTest = square(lposition[eBoundLoc0] / get(eInnerRx)) + + square(lposition[eBoundLoc1] / get(eInnerRy)); + + // Check if point is inside outer ellipse + double outerEllipseTest = square(lposition[eBoundLoc0] / get(eOuterRx)) + + square(lposition[eBoundLoc1] / get(eOuterRy)); + + return withinPhiSector && (innerEllipseTest >= 1) && (outerEllipseTest < 1);
46-59: Consider the path to public API, you must.Hmmmm. Wisdom share, I shall:
- Integration with AbsoluteBound and AbsoluteCartesian, careful consideration needs
- Consistent approach across all bounds classes, maintain we must
- Clear documentation of assumptions and limitations, essential it is
Suggest these steps, I do:
- Document assumptions about coordinate systems
- Add comprehensive tests before public API release
- Consider performance implications of metric calculations
Core/include/Acts/Surfaces/DiamondBounds.hpp (1)
79-81: New method for closest point calculations, approve I do. Yet tests we must write!Well-designed the new method is, with optional metric parameter included. But remember we must, untested code path to the dark side leads.
Help you write comprehensive tests for the closestPoint method, shall I? Generate test cases covering various scenarios, I can.
Core/include/Acts/Surfaces/InfiniteBounds.hpp (2)
27-40: Wise implementation, this is!Identity matrix for infinite bounds, most appropriate it is. Unused parameters, properly silenced they are.
Consider passing Vector2 by const reference in method signatures, memory efficiency it brings:
- SquareMatrix2 boundToCartesianJacobian(const Vector2& lposition) const final { + SquareMatrix2 boundToCartesianJacobian(const Vector2& lposition) const final {
Line range hint
1-83: Strong with the force, this implementation is!Well-designed interface for infinite bounds, you have created. Support boundary tolerance calculations effectively, it will. Maintain consistency with base class, it does. Clean and maintainable, the code is.
Core/src/Surfaces/TrapezoidBounds.cpp (2)
Line range hint
40-70: Approve this implementation, I do! Documentation, it needs though.Well-structured the method is, with early returns and proper geometric transformations. Efficient checks for y and x ranges before the more expensive polygon check, implemented you have.
Add documentation you should, explaining the coordinate transformation and boundary check strategy:
+/** + * Check if a point is inside the trapezoid bounds. + * @param lposition Position in local coordinates + * @return true if inside, false otherwise + * + * First transforms the point by rotation, then performs + * efficient range checks before falling back to full + * polygon containment test if needed. + */
72-88: Approve this implementation too, I do! But performance considerations, discuss we must.Elegant the solution is, with proper coordinate transformations and delegation to helper functions. However, cache the vertices we could, instead of recreating them on each call.
Consider this optimization, you should:
Vector2 TrapezoidBounds::closestPoint( const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const { const double hlXnY = get(TrapezoidBounds::eHalfLengthXnegY); const double hlXpY = get(TrapezoidBounds::eHalfLengthXposY); const double hlY = get(TrapezoidBounds::eHalfLengthY); const double rotAngle = get(TrapezoidBounds::eRotationAngle); const Vector2 extPosition = Eigen::Rotation2Dd(rotAngle) * lposition; - - Vector2 vertices[] = { - {-hlXnY, -hlY}, {hlXnY, -hlY}, {hlXpY, hlY}, {-hlXpY, hlY}}; + // Use cached vertices from the vertices() method + const auto& vertices = this->vertices(); Vector2 extClosest = detail::VerticesHelper::computeClosestPointOnPolygon( extPosition, vertices, metric.value_or(SquareMatrix2::Identity()));Core/src/Surfaces/LineBounds.cpp (1)
38-44: Document the ways of the Force, we must!Sound implementation this is, but documentation about metric parameter's purpose and usage, missing it is. Help future padawans understand this power, we should.
Add documentation like this, you should:
+ /// Compute the closest point on the bounds to a given position + /// @param lposition Local position to check against bounds + /// @param metric Optional metric for distance calculation (identity if not provided) + /// @return Vector2 The closest point on the bounds Vector2 LineBounds::closestPoint( const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const {Core/include/Acts/Surfaces/EllipseBounds.hpp (1)
79-81: New closestPoint method, essential for boundary calculations it is!Hmm, metric parameter optional it is, but documentation for its purpose, missing I see. Clarify the impact of metric parameter on calculations, we should.
Add documentation like this, you should:
+ /// Find the closest point on the bounds to a given position + /// @param lposition Local position to find closest point to + /// @param metric Optional metric to define distance (default: Euclidean) + /// @return The closest point on the bounds Vector2 closestPoint(const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const final;Core/src/Surfaces/RadialBounds.cpp (2)
46-54: Documentation for the metric matrix, add you should.Correct the implementation is, but mysterious to young padawans it may seem. A comment explaining the mathematical significance of this metric matrix, beneficial would be.
Add this documentation above the method:
/// Computes the metric matrix for polar coordinates /// This matrix represents the local metric tensor used for distance calculations /// @param lposition Local position in polar coordinates (r, phi) /// @return Metric matrix with diagonal elements (1, r²)
69-74: Strong with the Force, this implementation is!Elegant solution for closest point calculation, you have created. The optional metric parameter, flexibility it brings. Consider this method, a valuable addition to our public API in the future, it could be.
When ready for public API inclusion, document the performance characteristics and usage examples, you should. Help future padawans understand the wisdom of this approach, it will.
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (1)
81-85: Welcome the new closestPoint method, I do!Wise addition this is, for boundary tolerance calculations it helps. Yet, document its behavior we should.
Add documentation comments explaining:
- The meaning of the optional metric parameter
- The behavior when point lies inside vs outside bounds
- The relationship with distance calculation
+ /// Find the closest point on the bounds to a given position + /// @param lposition Local position to check against + /// @param metric Optional metric to use for distance calculation + /// @return The closest point on the boundsCore/include/Acts/Surfaces/RadialBounds.hpp (1)
77-80: Strong with the Force, this implementation is!Key addition for boundary calculations, this method is. But documentation for the metric parameter, improve we could.
Add parameter documentation like this, you should:
/// @copydoc SurfaceBounds::closestPoint + /// @param metric Optional metric for distance calculation in local coordinatesCore/include/Acts/Surfaces/TrapezoidBounds.hpp (3)
56-56: Documentation inheritance through @copydoc, wise choice it is.Clear documentation lineage through
@copydoc, maintain consistency it does. Yet, virtual method this is, document return value format we should.Add return value documentation:
/// @copydoc SurfaceBounds::values + /// @return Vector of bounds parameters {halfXnegY, halfXposY, halfY, rotationAngle} std::vector<double> values() const final;
99-101: New closestPoint method, essential for boundary calculations it is.Metric parameter optional it is, wise choice for flexibility. Yet documentation for metric usage, missing it remains.
Add parameter documentation:
/// @copydoc SurfaceBounds::closestPoint + /// @param metric Optional 2x2 metric tensor for distance calculation Vector2 closestPoint(const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const final;
97-102: Architectural direction, clear it becomes.Removal of BoundaryTolerance and addition of closestPoint, strategic steps they are. For edge hole determination in track finding, useful this shall be. Yet, consider these architectural points, we must:
- Separation of concerns: Distance computation from boundary checks, good practice it is
- Future extensibility: Inside check implementation, easier it becomes
- Performance impact: Metric parameter optional keeping, wise choice for optimization it is
Strong in the Force, this design is.
Core/src/Surfaces/CylinderBounds.cpp (1)
Line range hint
36-80: Simplify the 'inside' method further, perhaps you can.Consider refactoring complex calculations into helper functions, you might. Enhance readability, it would.
Core/include/Acts/Surfaces/AnnulusBounds.hpp (1)
196-201: Add documentation to new methods, you should.Improve maintainability by adding comments for 'stripPCToModulePC' and related methods, it will.
Core/include/Acts/Surfaces/LineBounds.hpp (1)
46-59: Wise implementation of Cartesian transformations, this is!Identity transformations for line bounds, correctly implemented they are. Clean and efficient the code is, with proper parameter handling through (void) casting.
Core/src/Surfaces/ConeBounds.cpp (1)
75-88: Efficient the implementation is, yet improve it we can!Good use of VerticesHelper for calculations, I see. But cache the r(lposition[1]) calculation, we should, as used twice it is.
Apply this optimization, you should:
bool ConeBounds::inside(const Vector2& lposition) const { - auto rphiHalf = r(lposition[1]) * get(eHalfPhiSector); + const auto rZ = r(lposition[1]); + auto rphiHalf = rZ * get(eHalfPhiSector); return detail::VerticesHelper::isInsideRectangle( shifted(lposition), Vector2(-rphiHalf, get(eMinZ)), Vector2(rphiHalf, get(eMaxZ))); } Vector2 ConeBounds::closestPoint( const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const { - auto rphiHalf = r(lposition[1]) * get(eHalfPhiSector); + const auto rZ = r(lposition[1]); + auto rphiHalf = rZ * get(eHalfPhiSector); return detail::VerticesHelper::computeClosestPointOnAlignedBox(Core/include/Acts/Surfaces/RectangleBounds.hpp (1)
74-76: Wise addition, the closestPoint method is!For boundary tolerance calculations, essential this method becomes. Optional metric parameter, flexibility it provides.
Consider documenting common metric configurations in the class documentation, help future users it will.
Tests/UnitTests/Core/Surfaces/SurfaceBoundsTests.cpp (2)
47-55: Approve the test implementations, I do!Simple yet sufficient for testing, these implementations are. Consider adding test cases with non-identity matrices, more thorough coverage it would provide.
59-73: Enhance test coverage, we should!Simple implementations provided are, but more realistic test scenarios benefit us they would. Add test cases with actual boundary calculations and point computations, I suggest.
Core/include/Acts/Surfaces/detail/VerticesHelper.hpp (1)
311-321: Optimization opportunity noted, hmm yes.Consider implementing direct computation for box with metric, more efficient it could be.
Help implement optimized computation for box with metric, shall I? Open issue to track this improvement, we can.
Core/include/Acts/Surfaces/Surface.hpp (1)
253-267: Enhance documentation clarity, young padawan must.Documentation improvements suggest I do:
/// Calculates the closest point on the boundary of the surface to a given /// point in local coordinates. /// @param lposition The local position to check /// @param metric The metric to use for the calculation + /// @return The closest point on the boundary in local coordinates virtual Vector2 closestPointOnBounds( const Vector2& lposition, const std::optional<SquareMatrix2>& metric) const; /// Calculates the distance to the boundary of the surface from a given point /// in local coordinates. /// @param lposition The local position to check + /// @return The signed distance to the boundary (negative if inside, positive if outside) virtual double distanceToBounds(const Vector2& lposition) const;Core/src/Surfaces/Surface.cpp (1)
249-257: Consider access level and documentation for internal API, we should.Internal use only, these methods are meant to be. Yet public they remain. Two paths forward, I see:
- Make protected these methods should be, if truly internal they are.
- Add documentation we must, clearly marking internal use only.
Choose wisely, young padawan.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp(0 hunks)Core/include/Acts/Surfaces/AnnulusBounds.hpp(2 hunks)Core/include/Acts/Surfaces/BoundaryTolerance.hpp(3 hunks)Core/include/Acts/Surfaces/ConeBounds.hpp(1 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp(3 hunks)Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp(5 hunks)Core/include/Acts/Surfaces/CylinderBounds.hpp(2 hunks)Core/include/Acts/Surfaces/DiamondBounds.hpp(1 hunks)Core/include/Acts/Surfaces/DiscBounds.hpp(1 hunks)Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp(1 hunks)Core/include/Acts/Surfaces/EllipseBounds.hpp(1 hunks)Core/include/Acts/Surfaces/InfiniteBounds.hpp(2 hunks)Core/include/Acts/Surfaces/LineBounds.hpp(1 hunks)Core/include/Acts/Surfaces/PlanarBounds.hpp(1 hunks)Core/include/Acts/Surfaces/RadialBounds.hpp(1 hunks)Core/include/Acts/Surfaces/RectangleBounds.hpp(1 hunks)Core/include/Acts/Surfaces/Surface.hpp(1 hunks)Core/include/Acts/Surfaces/SurfaceBounds.hpp(2 hunks)Core/include/Acts/Surfaces/TrapezoidBounds.hpp(2 hunks)Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp(0 hunks)Core/include/Acts/Surfaces/detail/VerticesHelper.hpp(2 hunks)Core/src/Detector/detail/CylindricalDetectorHelper.cpp(0 hunks)Core/src/Surfaces/AnnulusBounds.cpp(3 hunks)Core/src/Surfaces/BoundaryTolerance.cpp(1 hunks)Core/src/Surfaces/CMakeLists.txt(1 hunks)Core/src/Surfaces/ConeBounds.cpp(2 hunks)Core/src/Surfaces/ConvexPolygonBounds.cpp(2 hunks)Core/src/Surfaces/CylinderBounds.cpp(2 hunks)Core/src/Surfaces/DiamondBounds.cpp(3 hunks)Core/src/Surfaces/DiscTrapezoidBounds.cpp(2 hunks)Core/src/Surfaces/EllipseBounds.cpp(1 hunks)Core/src/Surfaces/LineBounds.cpp(2 hunks)Core/src/Surfaces/RadialBounds.cpp(1 hunks)Core/src/Surfaces/RectangleBounds.cpp(3 hunks)Core/src/Surfaces/Surface.cpp(1 hunks)Core/src/Surfaces/SurfaceBounds.cpp(1 hunks)Core/src/Surfaces/TrapezoidBounds.cpp(2 hunks)Tests/Benchmarks/BoundaryToleranceBenchmark.cpp(3 hunks)Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp(3 hunks)Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp(9 hunks)Tests/UnitTests/Core/Surfaces/SurfaceBoundsTests.cpp(1 hunks)Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp(3 hunks)
💤 Files with no reviewable changes (3)
- Core/include/Acts/Geometry/TrapezoidVolumeBounds.hpp
- Core/include/Acts/Surfaces/detail/BoundaryCheckHelper.hpp
- Core/src/Detector/detail/CylindricalDetectorHelper.cpp
✅ Files skipped from review due to trivial changes (1)
- Core/include/Acts/Surfaces/DiscBounds.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: linux_examples_test
- GitHub Check: linux_physmon
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: build_debug
- GitHub Check: missing_includes
🔇 Additional comments (79)
Core/src/Surfaces/BoundaryTolerance.cpp (1)
46-47: Approve this change, I do!Simplified the enum usage has been, through modern C++20 features. Cleaner and more concise, the code now is, hmm.
Core/include/Acts/Surfaces/BoundaryTolerance.hpp (3)
72-75: Wise validation added, it has been!Prevent inconsistent states, this check does. When one tolerance zero is while the other is not, catch it we will. Strong in the Force, this defensive programming is!
150-154: Simpler name chosen wisely, it was!Discussed in council this was, and agreement reached we did. Clear and concise, the new name is. Documentation preserved well, it has been.
169-170: Consistent with the Force, this change is!Aligned with the enum rename, the method declaration now is. Clear documentation remains, hmm.
Tests/Benchmarks/BoundaryToleranceBenchmark.cpp (1)
11-11: Wise choice, this include is! Hmmmm.A better path you have chosen, replacing BoundaryCheckHelper with ConvexPolygonBounds. Align with the force of encapsulation, this does.
Core/include/Acts/Surfaces/DiamondBounds.hpp (2)
68-69: Simplified enum access, approve I do!Cleaner the code becomes when directly accessing eDiamond we do. Consistent with other bounds classes this change is, hmmmm.
76-77: Simplified method signature, wisdom in this change I sense.Removed the BoundaryTolerance parameter we have, yet accessible the base class methods remain through the using declaration. Verify the impact across the codebase we must.
Also applies to: 83-83
✅ Verification successful
Safe the changes are, proceed we may.
Through the Force, searched the codebase I have. No direct calls to DiamondBounds::inside() with BoundaryTolerance found there were. Base class methods, accessible they remain through the using declaration. Test files and production code, through proper interfaces they work, hmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct calls to inside() with boundary tolerance rg "inside.*BoundaryTolerance" --type cppLength of output: 3249
Core/include/Acts/Surfaces/InfiniteBounds.hpp (5)
12-12: Hmmmm, proper include this is!Necessary for the boundary tolerance parameter, this header is. Follow the project's style guidelines, it does.
24-25: Clear as crystal, this change is!Explicit about the return type, we now are. Documentation from the base class, properly inherited it is.
45-49: Simple yet effective, this implementation is!For infinite bounds, always true it should return. Properly handled, the unused parameter is.
51-56: Align with the force, this implementation does!Support boundary tolerance calculations, this method will. For infinite bounds, return unchanged position it must. Handle unused metric parameter correctly, it does.
58-62: Consistent with the infinite nature, this implementation is!Always true it returns, as infinite bounds know no boundaries. Properly silenced, the unused parameters are.
Tests/UnitTests/Core/Surfaces/TrapezoidBoundsTests.cpp (2)
166-167: Comprehensive test coverage, you have achieved!Thorough verification of the inside check implementation, your tests provide. Both inside and outside points, you have tested wisely.
180-189: Missing tests for closestPoint method, I sense!While random testing for inside check, thorough it is, tests for the new closestPoint method, absent they are.
Add tests for closestPoint you must, with cases such as:
- Points inside the trapezoid
- Points outside on each edge
- Points at vertices
- Points far from the trapezoid
- Different metric configurations
Help you with test implementation, shall I?
Core/src/Surfaces/LineBounds.cpp (2)
11-11: Wise choice, the inclusion of VerticesHelper is! Hmmmm.A more elegant approach to geometric calculations, this header brings. Removed complexity of boundary tolerance, we have.
31-36: Simplified the path to truth is, young padawan!Clean and elegant, this implementation is. Rectangle bounds for line surface, wisely you use. Yet verify we must, that other components adapt to this change they do.
Run this script to check for any lingering references to the old method signature:
✅ Verification successful
Hmmmm... Safe this change is, young Padawan!
Through the Force, seen I have:
- Through polymorphic interface, all calls flow they do
- Direct usage of LineBounds::inside(), found none we have
- Test files only, the old signature they remember
Continue your path to simplified code, you may. The Force is strong with this change!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls to inside() with BoundaryTolerance rg -l "inside.*BoundaryTolerance" --type cppLength of output: 561
Script:
#!/bin/bash # Check context of inside() usage in non-test files echo "=== PlanarSurfaceMask.cpp ===" rg "inside.*BoundaryTolerance" Fatras/src/Digitization/PlanarSurfaceMask.cpp -B 2 -A 2 echo -e "\n=== InfiniteBounds.hpp ===" rg "inside.*BoundaryTolerance" Core/include/Acts/Surfaces/InfiniteBounds.hpp -B 2 -A 2 echo -e "\n=== Direct LineBounds usage ===" rg "LineBounds.*inside" --type cpp -B 2 -A 2Length of output: 1417
Core/include/Acts/Surfaces/SurfaceBounds.hpp (2)
14-15: Header Files Included AppropriatelyThe inclusion of
<cmath>and<optional>headers is appropriate, given the use ofstd::sqrtandstd::optionalin the code. This ensures that the mathematical calculations and optional parameters function correctly.
91-100: Verify the Correctness of thedistanceMethod ImplementationThe
distancemethod computes the distance using the metric and the closest point. Ensure that:
- The
boundToCartesianMetricmethod returns a positive-definite matrix to avoid issues with the square root of negative values.- The mathematical operations are accurately implemented.
These verifications will confirm the reliability and robustness of the distance calculation.
Core/src/Surfaces/CMakeLists.txt (1)
25-25:SurfaceBounds.cppAdded to Build ConfigurationThe source file
SurfaceBounds.cpphas been correctly added to thetarget_sourcesof theActsCoretarget. This ensures that the new implementations inSurfaceBounds.cppare included in the build process.Core/include/Acts/Surfaces/EllipseBounds.hpp (4)
68-69: Simplified type() method, approve I do!More concise the code now is, removing redundant namespace qualification. Cleaner and clearer, the intent becomes.
76-77: Removal of BoundaryTolerance parameter, wise decision it is!Aligns with the broader initiative to simplify boundary checks across surface classes, this change does. Consistent with other bounds classes like AnnulusBounds and ConeBounds, it now is.
83-83: Base class inside() method, accessible it remains!Wise decision to keep base class functionality through using declaration. Clear intent to preserve base class behavior, this shows.
76-83: Verify impact on edge holes during track finding, we must!According to PR objectives, these changes support edge holes detection during track finding. Verify this functionality, we should.
Run this verification script, we must:
Core/src/Surfaces/RadialBounds.cpp (2)
36-44: Correct, the Jacobian matrix implementation is, hmm!Properly derived from polar to Cartesian transformation, this Jacobian matrix is. Clear and mathematically sound, the implementation appears.
63-67: Simplified, the boundary check has become. Wise, this change is.Removed the boundary tolerance complexity has been, making the code clearer it does. Yet verify we must that this simplification affects not the edge cases.
✅ Verification successful
Safe, the removal of boundary tolerance is. Proven by the Force, it has been!
Through the ancient texts of tests, revealed it is that BoundaryTolerance::None() already in use was. No disturbance in the Force do we sense, as:
- Tests comprehensive they are, yet no dependency on tolerance they show
- Already expecting zero tolerance behavior, the existing tests were
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be affected by boundary tolerance removal rg -l "BoundaryTolerance.*RadialBounds.*inside" Core/test/Length of output: 111
Script:
#!/bin/bash # Find test files related to RadialBounds fd "RadialBounds.*Test" --type f # Search for boundary tolerance usage in RadialBounds context rg -i "RadialBounds.*boundary|boundary.*RadialBounds" -A 3 # Find all usages of inside method with RadialBounds ast-grep --pattern 'RadialBounds$_->inside($_)'Length of output: 705
Script:
#!/bin/bash # Examine RadialBounds test content cat Tests/UnitTests/Core/Surfaces/RadialBoundsTests.cpp # Find any other potential boundary checks rg "inside.*boundary" -A 3Length of output: 8201
Core/src/Surfaces/RectangleBounds.cpp (5)
11-12: Appropriate, the inclusion of headers is.Necessary for new functionalities, these headers are.
36-38: Implemented correctly, thevaluesmethod is.Returns minimum and maximum coordinates, it does.
49-51: Boundary tolerance removed, you have. Verify impact, we must.Ensure that the absence of
BoundaryToleranceininsidemethod affects functionality not, you should.
53-66: Well-designed, theclosestPointmethod is.Handles both metric and non-metric cases efficiently, it does.
63-64: Verify the initialization ofvertices, you should.Correct representation of rectangle's corners, ensure you must.
Core/include/Acts/Surfaces/DiscTrapezoidBounds.hpp (3)
62-63: Simplified type() method, approve I do!Cleaner and more concise the code becomes. Through namespace Acts, properly scoped the enum value remains.
78-79: Consider the impact of removing boundary tolerance, we should!Removed the boundary tolerance parameter has been, as part of larger simplification effort. Yet, verify we must that edge cases still handled correctly are.
For track finding and edge holes determination, important this change is. Consider documenting the new approach to boundary handling in the class documentation, you should.
65-72: Verify the implementation of Cartesian transformation methods, we must!Correctly declared the methods are, but their implementation see we must. Important for coordinate transformations, these methods are.
Run this script to find the implementation:
✅ Verification successful
Correct and complete, the Cartesian transformation implementations are!
In the source file, found the implementations we have. Proper mathematical transformations they contain:
- Jacobian matrix, the coordinate transformation derivatives it captures
- Metric tensor, the local geometry it describes
Strong with the Force, this code is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for implementation of boundToCartesianJacobian and boundToCartesianMetric rg -l "boundToCartesianJacobian.*DiscTrapezoidBounds" --type cpp rg -l "boundToCartesianMetric.*DiscTrapezoidBounds" --type cppLength of output: 131
Script:
#!/bin/bash # Check for DiscTrapezoidBounds implementation file fd "DiscTrapezoidBounds" Core/src/Surfaces/ # Look at the header file content for possible inline implementation cat Core/include/Acts/Surfaces/DiscTrapezoidBounds.hppLength of output: 5884
Script:
#!/bin/bash # Check implementation of Cartesian transformation methods cat Core/src/Surfaces/DiscTrapezoidBounds.cppLength of output: 4753
Core/include/Acts/Surfaces/RadialBounds.hpp (4)
58-60: Simplified enum usage, approve I do!Wise decision it is, to use
eDiscdirectly. Cleaner and more readable, the code becomes. Functionally equivalent, it remains.
81-81: Wise to preserve the base class interface, it is!Maintain access to SurfaceBounds::inside, this declaration does. Good practice, this is.
61-68: Verify the implementation of Cartesian transformations, we must!Added these methods are, but their implementation in source file, see we must. Critical for boundary tolerance calculations, they are.
✅ Verification successful
Correct and complete, these implementations are!
Proper mathematical transformations for polar coordinates, I sense. The Force flows strong through these functions:
- Jacobian matrix with trigonometric wisdom, it carries
- Metric tensor with radial scaling, it maintains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of these methods rg "RadialBounds::boundToCartesianJacobian|RadialBounds::boundToCartesianMetric" Core/src/Length of output: 268
Script:
#!/bin/bash # Get the implementation details with context rg "RadialBounds::boundToCartesianJacobian|RadialBounds::boundToCartesianMetric" Core/src/Surfaces/RadialBounds.cpp -B 2 -A 10Length of output: 706
74-76: Careful with boundary tolerance removal, we must be!Removed the boundary tolerance parameter has been. Impact on existing code, this might have. Verify all callers, we should.
✅ Verification successful
Safe, this change is. Part of a larger refactoring, it appears to be!
The removal of boundary tolerance parameter aligns with new
BoundaryToleranceclass implementation, it does. ThroughBoundaryTolerance::None(), the same functionality preserved is. Extensive test coverage, I see. Worry about this change, you need not.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to inside() method with boundary tolerance rg "inside\([^)]+,[^)]+\)" --type cppLength of output: 24563
Script:
#!/bin/bash # Check for BoundaryTolerance usage and SurfaceBounds interface echo "=== Checking BoundaryTolerance usage ===" rg "BoundaryTolerance::" --type cpp -A 2 echo -e "\n=== Checking SurfaceBounds interface ===" rg "class SurfaceBounds" --type hpp -A 10Length of output: 68963
Core/src/Surfaces/DiscTrapezoidBounds.cpp (4)
54-61: Implemented correctly, the Jacobian is.The
boundToCartesianJacobianmethod computes the Jacobian matrix properly. Verified, I have, and correct it is.
64-72: Accurate computation of metric matrix, I see.In
boundToCartesianMetric, the metric matrix you compute correctly. Approve, I do.
74-81: Proper use ofVerticesHelperininsidemethod, you have.The
insidefunction correctly utilizesVerticesHelper::isInsidePolygonto determine point inclusion. Good practice, this is.
83-93: Ensure metric handling inclosestPoint, you must.Optional metric parameter you use. If none provided, default to Identity matrix it does. Verify that this behavior aligns with intended usage, we should.
Test cases to confirm the correctness of
closestPoint, important they are. Consider adding unit tests for this method.Generate unit tests for
closestPoint, I can. Open a new issue to track this task, would you like me to?Core/include/Acts/Surfaces/TrapezoidBounds.hpp (3)
53-54: Simplified namespace reference, approve I do.Direct use of
eTrapezoidinstead ofSurfaceBounds::eTrapezoid, cleaner the code becomes. Less verbose, more maintainable, hmmmm.
103-103: Using declaration, prevent hiding of base class overloads it does.Proper inheritance practice this is, prevent shadowing of base class methods it does.
Line range hint
59-97: Extensive documentation for inside() method, valuable it is. Yet verify boundary cases, we must.Rich geometric explanation the documentation provides, hmmmm. But boundary cases, test we should.
Run this script to verify test coverage:
✅ Verification successful
Thorough test coverage for boundary cases, I sense. Worry not, we must.
The Force reveals comprehensive testing of boundary cases in TrapezoidBoundsTests.cpp:
- Invalid construction with negative values
- Edge points with and without rotation
- Strict boundary checks using BoundaryTolerance::None()
- Points exactly on boundaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for boundary case tests in the test files rg -l "TrapezoidBounds.*inside.*test" | while read -r file; do echo "=== $file ===" rg "TEST|BOOST_AUTO_TEST_CASE.*inside.*TrapezoidBounds" "$file" -A 10 doneLength of output: 54
Script:
#!/bin/bash # First find all test files related to TrapezoidBounds echo "=== Finding TrapezoidBounds test files ===" fd "Test.*\.cpp$|.*Tests\.cpp$" | rg -l "TrapezoidBounds" echo -e "\n=== Searching for inside() tests ===" # Search with more context and broader patterns rg "TEST.*Trapezoid|BOOST.*Trapezoid" -A 20 Tests/Length of output: 36014
Core/src/Surfaces/SurfaceBounds.cpp (1)
13-50: Implementation of the 'inside' method correct, it is.Handles boundary tolerances appropriately, the logic does. No issues seen, there are.
Core/src/Surfaces/CylinderBounds.cpp (1)
82-100: Correctly implemented, the 'closestPoint' method is.Logical and clear, the code appears. Approve this addition, I do.
Core/include/Acts/Surfaces/AnnulusBounds.hpp (5)
68-68: Properly updated, the 'type' method is.Simplifies type identification, this change does. Correct, it is.
70-72: Added 'isCartesian' method, you have.Enhances functionality, this does. Approve, I do.
73-77: Implemented coordinate transformation methods, you did.'boundToCartesianJacobian' and 'boundToCartesianMetric' valuable additions, they are.
84-85: Modified the 'inside' method appropriately, you have.Removal of 'BoundaryTolerance' parameter aligns with refactoring efforts, it does.
86-88: Valuable addition, the 'closestPoint' method is.Enhances point proximity calculations, it does. Good work, this is.
Core/src/Surfaces/AnnulusBounds.cpp (6)
174-182: Implemented correctly, theboundToCartesianJacobianmethod is.Accurately computes the Jacobian from polar to Cartesian coordinates it does.
186-192: Proper theboundToCartesianMetricmethod implementation is.Returns the metric tensor for polar coordinates it does, as expected.
194-213: Simplified theinsidemethod you have, hmmm.Unnecessary parameters removed are, clarity improved is.
217-298: Detailed derivation instripPCToModulePCJacobianvaluable it is.Computation of the Jacobian thorough and precise appears.
300-372: Well-structured theclosestPointmethod is, yes.Efficiently computes the closest point it does, using metrics appropriately.
376-390: Conversion methods between coordinate systems added you have.Consistency in transformations this ensures.
Core/include/Acts/Surfaces/PlanarBounds.hpp (3)
39-41: Wise addition theisCartesianmethod is.Correctly indicates the coordinate system it does.
42-47: Appropriate theboundToCartesianJacobianmethod implementation is.Returns the identity matrix, as in Cartesian coordinates we are.
48-53: Correct theboundToCartesianMetricmethod is.Identity matrix it returns, suitable for Cartesian bounds it is.
Core/src/Surfaces/ConvexPolygonBounds.cpp (3)
12-12: IncludingVerticesHelper.hppnecessary it is, hmmm.Provides required helper functions it does.
48-50: Simplified theinsidemethod you have.Uses
isInsidePolygon, efficient and clear it becomes.
52-57: Refactored theclosestPointmethod well you have.Utilizes
computeClosestPointOnPolygon, consistency this brings.Core/include/Acts/Surfaces/LineBounds.hpp (2)
43-44: Approve this change, I do!More efficient the direct return is, than through base class calling. Wisdom in simplicity there is.
68-71: Consistent with the ways of the Force, this addition is!Well-designed the closestPoint method is, with optional metric parameter it comes. Harmony with other surface bounds, it maintains.
Core/include/Acts/Surfaces/ConvexPolygonBounds.ipp (2)
15-16: Modern C++ wisdom, strong in this one is!Proper use of concepts for type constraints, I see. Clear error messages and strong type safety, this brings.
98-108: Consistent with the Jedi way, these implementations are!Well-implemented the geometric calculations are, using VerticesHelper they do. Consistent with other bounds classes, maintaining harmony in the codebase.
Core/include/Acts/Surfaces/RectangleBounds.hpp (3)
65-66: Approve this change, I do!Cleaner and more direct, the code becomes. Consistent with other bounds classes, this change is.
78-78: Approve this using declaration, I do!Both overloads accessible it makes. Consistent with the way of the bounds classes, this is.
71-72: Verify the impact of boundary tolerance removal, we must.Changed the signature has been, removing BoundaryTolerance parameter. Ensure that all callers updated are, and no regressions introduced have been.
✅ Verification successful
Safe the changes are, proceed you may.
Found one reference to BoundaryTolerance I have, but in InfiniteBounds class it resides, which from SurfaceBounds directly inherits. A separate implementation it maintains, affected by RectangleBounds changes it is not. No inheritance conflicts detected have I.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls to inside() with BoundaryTolerance rg -l "inside.*BoundaryTolerance" Core/include/Acts/Surfaces/Length of output: 109
Script:
#!/bin/bash # Check the content of InfiniteBounds.hpp cat Core/include/Acts/Surfaces/InfiniteBounds.hpp # Search for classes inheriting from RectangleBounds ast-grep --pattern 'class $_ : $$ RectangleBounds'Length of output: 2385
Tests/UnitTests/Core/Surfaces/SurfaceBoundsTests.cpp (1)
37-37: Well structured, the test class is!Complete interface implementation, provided it has. For thorough testing, essential this is.
Also applies to: 43-45
Core/include/Acts/Surfaces/ConvexPolygonBounds.hpp (3)
36-38: LGTM! Wise implementation of type() method, it is.Final keyword ensures no override possible, maintaining type safety it does.
100-107: Simplified interface for boundary checks, I see.Removed boundary tolerance parameter and added
closestPointmethod, aligning with PR objectives it does. Good abstraction through base class methods, it provides.
148-155: Consistent implementation in specialized class, hmm yes.Mirror changes from base implementation, maintaining consistency across classes it does.
Tests/UnitTests/Core/Surfaces/AnnulusBoundsTests.cpp (1)
233-241: Updated test expectations for boundary tolerance, wise it is.Changed assertions reflect new behavior for points near boundaries. Comprehensive test coverage, it provides.
Tests/UnitTests/Core/Surfaces/BoundaryToleranceTests.cpp (1)
283-288: Simplified test setup using ConvexPolygonBounds, good it is.Clear and concise test structure, it provides. Lambda function for checks, reusable it makes.
Core/include/Acts/Surfaces/detail/VerticesHelper.hpp (1)
301-310: Wise implementation of closest point computation, it is.Fallback to Euclidean computation when no metric provided, efficient it is.
Core/src/Surfaces/Surface.cpp (2)
249-253: Approve this implementation, I do.Wise delegation to bounds object, this is. Clean and simple, the implementation remains.
255-257: Approve this implementation too, I must.To bounds object, wisely delegates it does. Consistency with closestPointOnBounds, maintained it is.
Adds
Vector2 closestPoint(const Vector2& lposition)anddouble distance(const Vector2& lposition)toSurfaceBounds. These are the building blocks for the boundary tolerance but not exposed to the user. In some cases these come in very handy for example to decide on edge holes during track finding. For this reason I think they should be elevated to public API.Potentially the inside check should then be implemented on top of this newly exposed functionality.
blocked by
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
Release Notes
New Features
Improvements
Changes
BoundaryToleranceparameter from multiple surface bounds methodstoleranceMode()→mode())Refactoring
Deprecations
Performance
Compatibility